-
Notifications
You must be signed in to change notification settings - Fork 4.1k
support change ownship for SelectiveChannel #3198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
LGTM |
src/brpc/selective_channel.h
Outdated
| int AddChannel(ChannelBase* sub_channel, ChannelHandle* handle) { | ||
| return AddChannel(sub_channel, "", OWNS_CHANNEL, handle); | ||
| } | ||
| int AddChannel(ChannelBase* sub_channel, const std::string& tag, | ||
| ChannelHandle* handle) { | ||
| return AddChannel(sub_channel, tag, OWNS_CHANNEL, handle); | ||
| } | ||
| int AddChannel(ChannelBase* sub_channel, ChannelOwnership ownership, | ||
| ChannelHandle* handle) { | ||
| return AddChannel(sub_channel, "", ownership, handle); | ||
| } | ||
| int AddChannel(ChannelBase* sub_channel, const std::string& tag, | ||
| ChannelOwnership ownership, ChannelHandle* handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think AddChannel has a combinatorial explosion problem.
int AddChannel(SelectiveChannel::SubChannelOptions);Using the SubChannelOptions overloaded AddChannel function would be more suitable, as it offers better extensibility.
In the meantime, please update the comments for the AddChannel function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
89ea0d5 to
05d8424
Compare
|
LGTM |
1 similar comment
|
LGTM |
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
What is changed and the side effects?
Changed:
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: